Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[Terra-Dropdown] VO does not announce Expanded state and Index of list items while navigating through arrow keys #3805

Merged
merged 44 commits into from
Jul 17, 2023

Conversation

AH106586Harika
Copy link
Contributor

@AH106586Harika AH106586Harika commented May 19, 2023

Summary

[Terra-Dropdown] VO does not announce Expand/Collapse state and Index of list items while navigating through arrow keys
What was changed:
Added screenreader support to announce index and Expanded state for dropdown list items by adding aria-label property.

Expand.collapse_VOL.mov
Untitled.2.mp4
Untitled.1.mp4

This PR resolves:

UXPLATFORM-9058


Thank you for contributing to Terra.
@cerner/terra

@AH106586Harika AH106586Harika self-assigned this May 19, 2023
@AH106586Harika AH106586Harika requested a review from a team as a code owner May 19, 2023 17:10
@supreethmr
Copy link
Contributor

supreethmr commented May 22, 2023

We can have this change on PR : #3798
since to validate index changes you would need to focus behavior also fixed !!

packages/terra-dropdown-button/translations/de.json Outdated Show resolved Hide resolved
@@ -69,6 +78,7 @@ class DropdownList extends React.Component {
event.preventDefault();
} else if (keyCode === KeyCode.KEY_DOWN) {
if (!this.pressed) {
this.expanded = '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a unit test to see if this gets set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented.

@@ -1,6 +1,8 @@
# Changelog

## Unreleased
* Added
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Added
* Added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there are 2 Added headings and they can be combined together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

const ofText = this.props.intl.formatMessage({ id: 'Terra.dropdownButton.of' });
const totalItems = this.props.children.length;
let ariaLabel = null;
if (SharedUtil.isMac() && currentIndex === 1 && totalItems) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we will be removing aria-expanded on DropdownButton.jsx . we should remove this condition of MAC and make it consistent across all the screen readers and browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented and tested it is working as expected.

if (SharedUtil.isMac() && currentIndex === 1 && totalItems) {
ariaLabel = `${this.expanded} ${currentItemLabel} (${currentIndex} ${ofText} ${totalItems})`;
} else if (SharedUtil.isMac() && currentIndex !== 1 && totalItems) {
ariaLabel = `${currentItemLabel} (${currentIndex} ${ofText} ${totalItems})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ariaLabel = `${currentItemLabel} (${currentIndex} ${ofText} ${totalItems})`;
ariaLabel = `${currentItemLabel}, (${currentIndex} ${ofText} ${totalItems})`;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@AH106586Harika AH106586Harika changed the title [Terra-Dropdown] VO does not announce Index of list items while navigating through arrow keys [Terra-Dropdown] VO does not announce Expanded state and Index of list items while navigating through arrow keys Jun 6, 2023

it('should set the aria-label property to empty on keydown on Mac', () => {
// Set the mock implementation for isMac
SharedUtil.isMac.mockReturnValue(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the mock

Comment on lines 137 to 143
expect(firstListItemAriaLabelValue).toEqual(expectedAriaLabelValueInitial);

// Simulate keydown event
wrapper.instance().handleKeyDown(eventMock);
const updatedFirstListItemAriaLabelValue = wrapper.find('#firstItem').props()['aria-label'];
const expectedAriaLabelValue = '1st Option,(1 of 3)';
expect(updatedFirstListItemAriaLabelValue).toEqual(expectedAriaLabelValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put all the expect statements at the bottom for better readability.

Suggested change
expect(firstListItemAriaLabelValue).toEqual(expectedAriaLabelValueInitial);
// Simulate keydown event
wrapper.instance().handleKeyDown(eventMock);
const updatedFirstListItemAriaLabelValue = wrapper.find('#firstItem').props()['aria-label'];
const expectedAriaLabelValue = '1st Option,(1 of 3)';
expect(updatedFirstListItemAriaLabelValue).toEqual(expectedAriaLabelValue);
// Simulate keydown event
wrapper.instance().handleKeyDown(eventMock);
const updatedFirstListItemAriaLabelValue = wrapper.find('#firstItem').props()['aria-label'];
const expectedAriaLabelValue = '1st Option,(1 of 3)';
expect(firstListItemAriaLabelValue).toEqual(expectedAriaLabelValueInitial);
expect(updatedFirstListItemAriaLabelValue).toEqual(expectedAriaLabelValue);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented as suggested.

wrapper.instance().listRef = listRefMock;
const firstListItem = wrapper.find('#firstItem');
const firstListItemAriaLabelValue = firstListItem.props()['aria-label'];
const expectedAriaLabelValueInitial = `${translationsFile['Terra.dropdownButton.expanded']}1st Option,(1 of 3)`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to expectedAriaLabelInitialValue

Suggested change
const expectedAriaLabelValueInitial = `${translationsFile['Terra.dropdownButton.expanded']}1st Option,(1 of 3)`;
const expectedAriaLabelInitialValue = `${translationsFile['Terra.dropdownButton.expanded']}1st Option,(1 of 3)`;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@@ -89,4 +115,58 @@ describe('Dropdown List', () => {
);
expect(wrapper).toMatchSnapshot();
});

it('should set the aria-label property to empty on keydown on Mac', () => {
// Set the mock implementation for isMac
Copy link
Contributor

@sdadn sdadn Jul 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
// Set the mock implementation for isMac
// Sets the mock return value for isMac()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Copy link
Contributor

@sdadn sdadn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was about to approve until I noticed this one last thing!!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't have the translations at this time, we should only keep en.json and en-US.json. The remaining files will be provided by the translations team when they are ready.

Copy link
Contributor Author

@AH106586Harika AH106586Harika Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have received translations from translation team and I have added them in the translation files . TRANS-14545

@AH106586Harika
Copy link
Contributor Author

Sorry, I was about to approve until I noticed this one last thing!!

@AH106586Harika
Copy link
Contributor Author

AH106586Harika commented Jul 17, 2023

Sorry, I was about to approve until I noticed this one last thing!!

let ariaLabel = null;
if (SharedUtil.isMac()) {
if (currentIndex === 1 && totalItems) {
ariaLabel = `${this.expanded}${activeOption}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
ariaLabel = `${this.expanded}${activeOption}`;
ariaLabel = `${this.expanded}, ${activeOption}`;

Lets remove the special characters like dot and comma from translations and include it as part of strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ariaLabel = `${this.expanded}${currentItemLabel}`;
} else if (currentIndex !== 1 && totalItems) {
ariaLabel = `${currentItemLabel}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we simplify this :

const activeOption = (SharedUtil.isMac()) ? this.props.intl.formatMessage({ id: 'Terra.dropdownButton.activeOption' }, { currentItemLabel, currentIndex, totalItems }) : currentItemLabel;
      let ariaLabel = '';
     if (currentIndex === 1 && totalItems) {
        ariaLabel = `${this.expanded}, ${activeOption}`;
      } else if (currentIndex !== 1 && totalItems) {
        ariaLabel = activeOption;
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified for better readability.

@@ -1,4 +1,4 @@
{
"Terra.dropdownButton.moreOptions": "More Options",
"Terra.dropdownButton.selected": "Valittu."
"Terra.dropdownButton.selected": "Valittu"
Copy link
Contributor

@supreethmr supreethmr Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are missing translations here !! Please make sure translations are added from valid file it should be from fi-FI.json file provided by translation team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't received translations for fi-FI.json when I checked with the translation team she told it is not a supported locale.

@github-actions github-actions bot temporarily deployed to preview-pr-3805 July 17, 2023 12:38 Destroyed
@saket2403 saket2403 merged commit c6b9a36 into main Jul 17, 2023
@saket2403 saket2403 deleted the AH106586/Dropdown-announce-index branch July 17, 2023 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants